Skip to content

TYP: enable reportPropertyTypeMismatch #43853

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 29, 2021
Merged

TYP: enable reportPropertyTypeMismatch #43853

merged 2 commits into from
Oct 29, 2021

Conversation

twoertwein
Copy link
Member

This might be controversial. The setter allows any Mapping but the getter returns a dict.

reportPropertyTypeMismatch:

[...] type of the value passed to the setter is not assignable to the value returned by the getter. Such mismatches violate the intended use of properties, which are meant to act like variables. The default value for this setting is 'error'.

@simonjayhawkins simonjayhawkins added the Typing type annotations, mypy/pyright type checking label Oct 3, 2021
@jreback jreback added this to the 1.4 milestone Oct 3, 2021
@@ -340,7 +340,7 @@ def attrs(self) -> dict[Hashable, Any]:
return self._attrs

@attrs.setter
def attrs(self, value: Mapping[Hashable, Any]) -> None:
def attrs(self, value: dict[Hashable, Any]) -> None:
self._attrs = dict(value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't you leave the input annotation and then this is equiv of a cast? we do this in lots of places. why is this flagged?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a typing issue, If I understand it correctly, pyright flags this because the property is meant to act like a variable, which means that setting and getting the value of the variable should have the same type.

Pyright doesn't like the following (independent of the function body):

@property
def attrs(self) -> dict[Hashable, Any]:
    ...

@attrs.setter
def attrs(self, value: Mapping[Hashable, Any]) -> None:
    ...

Both should be a Mapping or both should be a dict.

I can see why pyright wants to encourage using property like a variable, but I'm also happy to close this PR if this is too controversial/not intended.

@jbrockmendel
Copy link
Member

This looks like a case where 1) pyright is technically incorrect (passing a non-dict Mapping looks benign) and 2) its not that big a deal (if using "dict" will allow us to enable pyright in a way that will prevent bugs, then its fine)

@twoertwein
Copy link
Member Author

twoertwein commented Oct 26, 2021

I think the three options are:

  1. Add a pyright ignore comment to the top of the file and enable the pyright rule or
  2. Keep the pyright rule disabled or
  3. Change Mapping to dict and enable the pyright rule

I'm slightly inclined to 1 as it seems to be a deliberate choice to support setting any dict-like object.

@twoertwein
Copy link
Member Author

I updated the PR to not change the type annotation, but instead add a pyright-ignore comment.

@jreback jreback merged commit a4bf5c3 into pandas-dev:master Oct 29, 2021
@twoertwein twoertwein deleted the property branch October 31, 2021 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Typing type annotations, mypy/pyright type checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants